Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netstat: Fix duplicate keys in libxo output #1331

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

bramton
Copy link
Contributor

@bramton bramton commented Jul 17, 2024

Resolves Bug 254672 and Bug 244589

@bramton bramton changed the title Fixed duplicate shutdown-timer appearing in the timeouts section of sctp [netstat] Fixed duplicate keys in libxo output Jul 17, 2024
bramton added a commit to bramton/freebsd that referenced this pull request Jul 19, 2024
Duplicate keys in libxo output have been given a unique name.

PR: 244589, 254672
Pull Request: freebsd#1331
@bramton bramton changed the title [netstat] Fixed duplicate keys in libxo output netstat: Fix duplicate keys in libxo output Jul 19, 2024
Copy link
Contributor

@gmshake gmshake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with the change shutdown-ack-timer. That can be committed fast if you want ( and I'd recommend you ) to split this change into two parts.

@@ -711,7 +711,7 @@ sctp_stats(u_long off, const char *name, int af1 __unused, int proto __unused)
"secret\n");
p(sctps_timopathmtu, "\t\t{:pmtu-timer/%ju} "
"{N:/PMTU timer%s fired}\n");
p(sctps_timoshutdownack, "\t\t{:shutdown-timer/%ju} "
p(sctps_timoshutdownack, "\t\t{:shutdown-ack-timer/%ju} "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

usr.bin/netstat/if.c Show resolved Hide resolved
bramton added a commit to bramton/freebsd that referenced this pull request Jul 23, 2024
The current libxo output uses the "dropped-packets" key for both incoming and
outgoing packets. This commit distinguishes between the two by adding an "in"
and "out" suffix. The original "dropped-packets" key is kept for backwards
compatibility for now.

PR: 244589
Pull request: freebsd#1331
bramton added a commit to bramton/freebsd that referenced this pull request Jul 23, 2024
The key "shutdown-timer" was present twice in the libxo output. The second
occurence of "shutdown-time" has been renamed to "shutdown-ack-timer".

PR: 254672
Pull request: freebsd#1331
@bramton
Copy link
Contributor Author

bramton commented Jul 23, 2024

Thanks for reviewing. I have split the commit into two seperate commits.

@@ -501,8 +501,11 @@ intpr(void (*pfunc)(char *), int af)
IFA_STAT(ipackets), link|network, 1);
show_stat("lu", nerr_len, "received-errors", IFA_STAT(ierrors),
link, 1);
/* Below is kept for backwards compatibility. Will be removed in the future. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to s/in the future/before FreeBSD 16/

@bsdimp
Copy link
Member

bsdimp commented Aug 23, 2024

And question: do we need to bump the version for this? libxo man pages are kinda silent about when to do that.

Copy link
Contributor

@gmshake gmshake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@gmshake
Copy link
Contributor

gmshake commented Aug 26, 2024

And question: do we need to bump the version for this? libxo man pages are kinda silent about when to do that.

@bsdimp IMO, for the fixes, dropped-packets-out and shutdown-ack-timer, no need to bump the version.
But for the new introduced dropped-packets-in I guess it will be easy to track the change if bump the version.

bramton added a commit to bramton/freebsd that referenced this pull request Aug 27, 2024
The key "shutdown-timer" was present twice in the libxo output. The second
occurence of "shutdown-time" has been renamed to "shutdown-ack-timer".

PR: 254672
Pull request: freebsd#1331
@bramton
Copy link
Contributor Author

bramton commented Aug 27, 2024

And question: do we need to bump the version for this? libxo man pages are kinda silent about when to do that.

netstat didn't have version information yet. This is added in 0d2e74b.

bramton and others added 4 commits September 20, 2024 09:06
The current libxo output uses the "dropped-packets" key for both incoming and
outgoing packets. This commit distinguishes between the two by adding an "in"
and "out" suffix. The original "dropped-packets" key is kept for backwards
compatibility for now.

PR: 244589
Reviewed by: imp, zlei
Pull Request: freebsd#1331
The key "shutdown-timer" was present twice in the libxo output. The second
occurence of "shutdown-time" has been renamed to "shutdown-ack-timer".

PR: 254672
Reviewed by: imp, zlei
Pull Request: freebsd#1331
NDFREE_PNBUF should be called after checking the return value of
vn_open(), and should only be called once.

Reviewed by: imp, zlei, Kornel Dulęba <[email protected]>, Elliott Mitchell
Pull Request: freebsd#1338
i2c -sv or i2c -rv prints an uninitialized field i2c_opt.addr.
Suppress the verbose message entirely for scan and reset,
where it provides no information, and zero initialize the field.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279261

Reviewed by: imp
Pull Request: freebsd#1345
@freebsd-git freebsd-git merged commit 14980d6 into freebsd:main Sep 20, 2024
21 of 23 checks passed
freebsd-git pushed a commit that referenced this pull request Sep 22, 2024
…utput"

This reverts commit 94567c8.

This breaks normal netstat output. It's kinda tricky to do a quick fix
on, so revert this until that can be fixed. If via github, please
open a new pull request...

Sponsored by:		Netflix
Pull Request: #1331
@bsdimp
Copy link
Member

bsdimp commented Sep 22, 2024

I've had to (partially) revert this pull request. For the bits I reverted, please resubmit as a new PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants